-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make CI deterministic try 2 #4190
Conversation
Yoooooo! The reference test is finally deterministic! It finally works! I'm going to polish this PR tomorrow. |
Alright, done. The output of JS code gen is finally deterministic. The causeThe chain that caused the nondeterministic code gen is as follows:
So what are import IDs? Import IDs are the arena ID from walrus' Since these IDs are nondeterministic, I suspect that imports are added into the arena in a nondeterministic order. However, I have no idea why that happens. My guess is that walrus just adds them in whatever order they appear in the WASM binary. This would explain why the order seems to depend on compiler versions and OS, and why #4138 (where I made everything in wasm bindgen deterministic) didn't work. Honestly, I'm not sure what the bug here is. Is the bug that import IDs are nondeterministic, or that wasm bindgen relied on them being deterministic? The fixThe fix I went with is quite simple: sort imports by name and not ID. Since the non-determinism of import IDs "infects" adapter IDs, I made sure that code gen doesn't depend on their order by sorting imports (and adapters of imports) by import name. This works, but isn't ideal IMO. I would have preferred to make adapter IDs deterministic, but I couldn't. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you for digging into this!
For future archeologists clarity:
wasm-bindgen
s ouput is already deterministic, just not across different versions of wasm-bindgen
, Rust and the OS.
This doesn't fix any bug and has no user-facing effects apart of the ordering of imported functions in the JS shim.
In the end, this is just to make integration tests in wasm-bindgen
less annoying by not having to re-order imports all the time.
Again, thank you for figuring this out! |
Continuation of #4138.